Add kwargs to every method in AIDA and test that implementations allow kwargs #75
Conversation
| that are not defined in the protocol. | ||
| """ | ||
| # Run in a temperature directory since we are saving an image | ||
| os.chdir(tmp_path) |
There was a problem hiding this comment.
This might leave the test suite in a "dirty state" for subsequent tests. You need a fixture to change back to test dir. Something similar to this:
There was a problem hiding this comment.
Ah; it turns out I can save the file to a temporary directory without needing to cd, so I changed the PR to do that.
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
Thanks!
I have comment on get but not sure if I left them at the correct place. Hope it is not too confusing.
| } | ||
|
|
||
| def get_stretch(self, image_label: str | None = None) -> BaseStretch: | ||
| def get_stretch( |
There was a problem hiding this comment.
Do get stuff really need **kwargs? I thought we want flexibility to set things but get should be straightforward? Am I missing something?
Same comment on all the other get methods.
There was a problem hiding this comment.
I wondered the same thing....definitely get_catalog_names and get_image_labels will get turned into properties without an arguments once this is merged.
| self, | ||
| sky_or_pixel: str | None = None, | ||
| image_label: str | None = None, | ||
| **kwargs, # noqa: ARG002 |
There was a problem hiding this comment.
That said, maybe we do want **kwargs here since viewer in Jdaviz has reference name as such, that might not apply to other backends.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the ImageViewer API to allow arbitrary keyword arguments on all methods and adds a test to verify that change.
- Extended every protocol method to accept
**kwargsand updated implementations accordingly - Added test in
api_test.pyto ensure all methods accept extra kwargs
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/astro_image_display_api/interface_definition.py | Add **kwargs to all protocol method signatures and update docstrings |
| src/astro_image_display_api/image_viewer_logic.py | Propagate **kwargs through concrete implementations with # noqa |
| src/astro_image_display_api/api_test.py | New test test_all_methods_accept_additional_kwargs |
Comments suppressed due to low confidence (3)
src/astro_image_display_api/interface_definition.py:189
- The docstring entry for
image_labelis missing its type annotation (str | None, optional). Please update it toimage_label : str | None, optionalto match the method signature.
image_label :
src/astro_image_display_api/interface_definition.py:43
- In the NumPy-style docstring
Parameterssection, the added**kwargsentry should include a type annotation and colon (e.g.,**kwargs : Any) to clearly document the accepted keyword arguments.
**kwargs
src/astro_image_display_api/interface_definition.py:161
- The docstring parameter name
kwargsdoes not match the signature**kwargsand lacks a type. It should be updated to**kwargs : Anyto remain consistent and clear.
kwargs :
2 participants
No description provided.